Skip to content

Rounding and billing fix#90

Merged
RuoHan-Chen merged 12 commits into
mainfrom
roundingAndBillingFix-contract-only
Jul 16, 2026
Merged

Rounding and billing fix#90
RuoHan-Chen merged 12 commits into
mainfrom
roundingAndBillingFix-contract-only

Conversation

@RuoHan-Chen

@RuoHan-Chen RuoHan-Chen commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Replace basis-points-encoded fees (feeBps) with an absolute feeAmount (in raw token units) on capture() and charge(). The payer-approved minFeeBps / maxFeeBps on PaymentInfo are unchanged and continue to bound the fee, but they are now applied to the operator-supplied absolute amount rather than used to compute it:

minFee = amount * minFeeBps / 10_000
maxFee = amount * maxFeeBps / 10_000
require(minFee <= feeAmount <= maxFee)

This lets operators pass cent-aligned fees computed off-chain without losing precision to on-chain BPS truncation, while preserving the same payer-side guarantees.

Design doc: https://docs.google.com/document/d/1RCIXH0Q46XcwyYcu3027g9bKXHljrEX-y_ciBIdGNIQ

Implementation follows the approach used in PPS.

Changes

Contracts

  • AuthCaptureEscrow.capture(...) and charge(...): uint16 feeBpsuint256 feeAmount.
  • _validateFee now takes amount + absolute feeAmount and checks it against the min/max bounds derived from bps.
  • _distributeTokens uses the supplied feeAmount directly (no on-chain multiplication/division).
  • Error FeeBpsOutOfRange(feeBps, minFeeBps, maxFeeBps)FeeAmountOutOfRange(feeAmount, minFee, maxFee).
  • PaymentCharged / PaymentCaptured events: uint16 feeBpsuint256 feeAmount.
  • Collector call sites (ERC3009PaymentCollector, Permit2PaymentCollector) updated to the new signature.

Docs

  • docs/Fees.md, docs/operations/Capture.md, docs/operations/Charge.md rewritten to describe absolute-fee semantics with worked examples and updated error list.

Tooling

  • Added [profile.ci] to foundry.toml.

Testing

Tests updated across the suite to pass absolute feeAmount values instead of bps:

  • test/src/PaymentEscrow/{authorize,capture,charge,refund,reentrancy,e2eCoinbaseSmartWallet}.t.sol
  • test/src/collectors/SpendPermissionPaymentCollector.t.sol
  • test/gas/gasBenchmark.t.sol
  • test/base/AuthCaptureEscrowBase.sol, test/base/AuthCaptureEscrowSmartWalletBase.sol

FeeAmountOutOfRange bounds cases (below-min, above-max, and both-zero) are exercised in capture.t.sol / charge.t.sol.

Backwards compatibility

Breaking public ABI change: charge(), capture(), PaymentCharged, and PaymentCaptured all change parameter/field types. Integrators must migrate before upgrading.

@cb-heimdall

cb-heimdall commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 1
Sum 2

⚠️ Ignored Reviews (1)

Reviewer Reason
stevieraykatz Soft review, as user does not have write permissions

@ilikesymmetry ilikesymmetry left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not review tests, but looks like a relatively simple modification. Good principle learned: prefer custom net-amount args as calldata with math + checks against constraints versus passing in fields that derive the net-amount and restrict rounding preferences.

Comment thread src/AuthCaptureEscrow.sol Outdated
Comment thread src/interfaces/IMulticall3.sol Outdated
Comment thread src/collectors/ERC3009PaymentCollector.sol Outdated
Comment thread src/AuthCaptureEscrow.sol
@RuoHan-Chen
RuoHan-Chen marked this pull request as ready for review June 25, 2026 22:48
Comment thread test/src/PaymentEscrow/charge.t.sol
Comment thread test/src/PaymentEscrow/capture.t.sol
Comment thread test/src/PaymentEscrow/capture.t.sol
Comment thread test/src/PaymentEscrow/charge.t.sol Outdated
Comment thread test/src/PaymentEscrow/capture.t.sol
@RuoHan-Chen
RuoHan-Chen force-pushed the roundingAndBillingFix-contract-only branch from 45b0c67 to b6e1478 Compare July 15, 2026 23:13
Add happy-path tests for feeAmount at the min/max bps-derived bounds
and for minFee=maxFee=0, a cent-aligned fee happy path for charge()
(mirroring capture's), and fuzz tests over feeAmount for both capture
and charge. Also rename test_charge_reverts_whenFeeAmountAboveMax to
test_reverts_whenFeeAmountAboveMax for naming consistency.
Complements the existing zero-fee happy path with the deterministic
revert case at the same boundary, since the fuzzed above-max test
only hits minFeeBps=maxFeeBps=0 probabilistically.

@amiecorso amiecorso left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀🚀🚀

env:
FOUNDRY_PROFILE: ci
BASE_RPC_URL: "https://mainnet.base.org"
BASE_RPC_URL: "https://base.publicnode.com"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something feels off about this. Are we getting rate limited on our own node for our CI? Can we confirm security is okay with this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah we were

@cb-heimdall

Copy link
Copy Markdown
Collaborator

Review Error for ilikesymmetry @ 2026-07-16 17:33:35 UTC
User failed mfa authentication, see go/mfa-help

@ilikesymmetry

Copy link
Copy Markdown
Collaborator

Think my phone OS is out of date, one sec

@RuoHan-Chen
RuoHan-Chen merged commit 6b0a5ef into main Jul 16, 2026
6 checks passed
@RuoHan-Chen
RuoHan-Chen deleted the roundingAndBillingFix-contract-only branch July 16, 2026 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants